fix(vm): guard VM stack against underflow #860
Conversation
Add explicit stack length checks in pop and current helpers. Extend VM tests to assert safe underflow error reporting. Signed-off-by: Ville Vesilehto <ville@vesilehto.fi>
|
But what is the case? Those opcode seq will not be generated? Only manually. |
|
Yes, the compiler output doesn't generate those sequences. But |
| if len(vm.Stack) == 0 { | ||
| panic("stack underflow") | ||
| } | ||
| return vm.Stack[len(vm.Stack)-1] |
There was a problem hiding this comment.
But this will also panic (with differnt message obviosly)
There was a problem hiding this comment.
Yeah, it's then easier to assert to that consistent error msg (if wanted). Maybe "robust" would have been a better choice than "safer".
Also just realised that this might become handy if the project fuzzer would do bytecode fuzzing as well. Distinguishing stack underflow from other errors.
There was a problem hiding this comment.
I wrote a small bytecode fuzzer and found other similar control flow edge cases. While the compiler never emits negative offsets for jump opcodes like OpJump and OpJumpIfTrue, the VM will happily take those in if defined as program bytecode. I was able to generate an infinite loop quite easily.
Example:
OpInt // arg: ...
OpInt // arg: ...
OpJump // arg: -N (negative offset)
OpInt
OpJump
Fix is simple:
func (vm *VM) Run(program *Program, env any) (_ any, err error) {
...
for vm.ip < len(program.Bytecode) {
case OpJump:
if arg < 0 {
panic("negative jump offset is invalid")
}
vm.ip += arg
...I can certainly open PRs to fix these, if we find this type of VM hardening feasible.
There was a problem hiding this comment.
I’m thinking of what benefits other than more readable error messages those checks can bring
2 participants
Add explicit stack length checks in
popandcurrentVM helpers. Extend VM tests to assert safe underflow error reporting.The test fails against
masteras follows:Bench results from
go test -run=^$ -bench=. -count=10 .below. Geomean at roughly +2%.